Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use git sha1 instead of counter tagging #121

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RuoqingHe
Copy link

@RuoqingHe RuoqingHe commented Jan 17, 2025

Summary of the PR

  • docker.sh: Reuse code as much as possible

  • docker.sh: Replace counter tagging with git sha1:

    We were using vXX counter tagging till v49 for x86_64 and aarch64,
    and v49-riscv for riscv64. And we have separated riscv64 from the
    other two to another job due to insufficient Github Action time limit
    (if the job failed to complete in 6 hours, it will be canceled), which
    end up of having two jobs. If the x86_64 and aarch64 job are finished
    and images are published in vN, while riscv64 job fails in that run,
    restarting the riscv64 job will result an absence of vN and a presence
    of v(N+1). So we decided to move on to git sha1 tagging strategy.

    Remove latest function and vXX counter tagging, and use output of

    git show -s --format=%h

    as the tag of images built per PR for publishing.

  • docker.sh: Introduce RISC-V support for manual operations

  • ci: Add latest tag for ease of use

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for implementing my suggestion of using sha1 instead of counter.
I suggest putting some context in the PR description and also in the commit description where we change from vXX to gYYYYY, especially to explain the reason of the changes, otherwise can be hard for reviewer to understand what we are fixing here.

What about updating the README.md too, where we used v38 as an example?

Maybe we can also add a section to mention that we used vXX till v49/v50, then switched to gYYYYY (where YYYYY=$(git show -s --format=%h))

docker.sh Outdated Show resolved Hide resolved
@RuoqingHe
Copy link
Author

What about updating the README.md too, where we used v38 as an example?

Would it be better to use latest tag in our README.md for developers who are going to use that image 🤔

@stefano-garzarella
Copy link
Member

What about updating the README.md too, where we used v38 as an example?

Would it be better to use latest tag in our README.md for developers who are going to use that image 🤔

This is a good idea, is latest automatically handled by dockerhub, or we need to push that tag every time?
BTW, I'd mention both gYYYYY and latest, mentioning the latest should not be used in rust-vmm-ci for the reasons we discussed on slack

@RuoqingHe
Copy link
Author

What about updating the README.md too, where we used v38 as an example?

Would it be better to use latest tag in our README.md for developers who are going to use that image 🤔

This is a good idea, is latest automatically handled by dockerhub, or we need to push that tag every time? BTW, I'd mention both gYYYYY and latest, mentioning the latest should not be used in rust-vmm-ci for the reasons we discussed on slack

Yes, gYYYY for rust-vmm-ci and latest for users, but I didn't find latest tag on our dockerhub. I'll dive to that action to see how to get it working 🙂

@RuoqingHe RuoqingHe force-pushed the use-git-sha1-instead-of-explict-versioning branch from 436a027 to f6da42c Compare January 17, 2025 10:30
docker.sh Outdated Show resolved Hide resolved
docker.sh Outdated Show resolved Hide resolved
docker.sh Outdated Show resolved Hide resolved
Currently, there are places hard-coded and functions manually
re-implemented, refactor `docker.sh` to a cleaner manner.

Signed-off-by: Ruoqing He <[email protected]>
@RuoqingHe RuoqingHe force-pushed the use-git-sha1-instead-of-explict-versioning branch from f6da42c to 4383699 Compare January 17, 2025 14:38
We were using `vXX` counter tagging till `v49` for x86_64 and aarch64,
and `v49-riscv` for riscv64. And we have separated riscv64 from the
other two to another job due to insufficient Github Action time limit
(if the job failed to complete in 6 hours, it will be canceled), which
end up of having two jobs. If the x86_64 and aarch64 job are finished
and images are published in vN, while riscv64 job fails in that run,
restarting the riscv64 job will result an absence of vN and a presence
of v(N+1). So we decided to move on to git sha1 tagging strategy.

Remove `latest` function and `vXX` counter tagging, and use output of

```console
git show -s --format=%h
```

as the tag of images built per PR for publishing.

Signed-off-by: Ruoqing He <[email protected]>
Add code for RISC-V specific tagging.

Signed-off-by: Ruoqing He <[email protected]>
@RuoqingHe RuoqingHe force-pushed the use-git-sha1-instead-of-explict-versioning branch from 4383699 to f500dde Compare January 17, 2025 15:00
@stefano-garzarella
Copy link
Member

LGTM! @RuoqingHe I'd just suggest fixing also the README and add a little section that we switched from vXX to gYYYYY.

`dev` image for x86_64 and aarch64 could be pulled through command
`docker pull rustvmm/dev:latest` and for riscv64 through `docker pull
rustvmm/dev:latest-riscv` for developers/users to use/test.

Signed-off-by: Ruoqing He <[email protected]>
@RuoqingHe RuoqingHe force-pushed the use-git-sha1-instead-of-explict-versioning branch from 24644a9 to 0c21d2c Compare January 20, 2025 10:58
@RuoqingHe
Copy link
Author

LGTM! @RuoqingHe I'd just suggest fixing also the README and add a little section that we switched from vXX to gYYYYY.

Thanks for reviewing, I gather there were something wrongly configured in my workflow, I force pushed to see if it's addressed 🙂

@RuoqingHe
Copy link
Author

LGTM! @RuoqingHe I'd just suggest fixing also the README and add a little section that we switched from vXX to gYYYYY.

Looks like that's now working, I will write them into README.md and document this change

Update `README.md` after we swtiched to `gYYYYY` git sha1 tagging from
`vXX` counter tagging.

Change example to pull an evolving `latest` for x86_64, aarch64 and
`latest-riscv` for riscv64.

Signed-off-by: Ruoqing He <[email protected]>
@RuoqingHe RuoqingHe changed the title Use git sha1 instead of explict versioning Use git sha1 instead of counter tagging Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants